-
Notifications
You must be signed in to change notification settings - Fork 58
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
README.md: fix build/run instructions #250
Conversation
Fix build and run instructions, put information at the top.
Given your tool shared library `<name_of_tool_shared_library>.so` (which contains kokkos profiling callback functions) and an application executable called yourApplication.exe, type: | ||
|
||
`export KOKKOS_TOOLS_LIBS=${YOUR_KOKKOS_TOOLS_DIR}/<name_of_tool_shared_lib>; ./yourApplication.exe` | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For those who also wondered what changed:
--- before
+++ after
@@ -1,19 +1,27 @@
-## Using cmake
+## Using cmake
+### Build
+
1. create a build directory in Kokkos Tools, e.g., type `mkdir myBuild; cd myBuild`
-2. To configure the Type `ccmake ..` for any options you would like to enable/disable.
+2. To configure the Type `ccmake .. -DCMAKE_INSTALL_PREFIX=` for any options you would like to enable/disable.
3. To compile, type `make`
4. To install, type `make install`
-## Using make
+### Run
-To build with make, simply type `make` within each subdirectory of Kokkos Tools.
+Given your installed tool shared library `lib<name_of_tool_shared_lib>.so` and an application executable called yourApplication.exe, type:
+`export KOKKOS_TOOLS_LIBS=${YOUR_KOKKOS_TOOLS_INSTALL_DIR}/lib<name_of_tool_shared_lib>.so; ./yourApplication.exe`
-Building using `make` is currently recommended. Eventually, the preferred method of building will be `cmake`.
-# Running a Kokkos-based Application with a tool
+## Using make
-Given your tool shared library `<name_of_tool_shared_library>.so` (which contains kokkos profiling callback functions) and an application executable called yourApplication.exe, type:
+### Build
-`export KOKKOS_TOOLS_LIBS=${YOUR_KOKKOS_TOOLS_DIR}/<name_of_tool_shared_lib>; ./yourApplication.exe`
+To build some library `<name_of_tool_shared_lib>` with make, simply type `make` within that library's subdirectory `${YOUR_KOKKOS_TOOLS_LIB_SRC_DIR}` of Kokkos Tools. This generate the shared library within that subdirectory.
+
+### Run
+
+Given your installed tool shared library `<name_of_tool_shared_lib>.so` and an application executable called yourApplication.exe, type:
+
+`export KOKKOS_TOOLS_LIBS=${YOUR_KOKKOS_TOOLS_LIB_SRC_DIR}/<name_of_tool_shared_lib>.so; ./yourApplication.exe`
When you make changes and craft commits, please think about the audit-ability to help reviewers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't add trailing whitespaces (after the cmake header for instance)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, got it and thanks.
I will see if I can make the changes clean.
README.md
Outdated
### Build | ||
|
||
1. create a build directory in Kokkos Tools, e.g., type `mkdir myBuild; cd myBuild` | ||
2. To configure the Type `ccmake .. -DCMAKE_INSTALL_PREFIX=` for any options you would like to enable/disable. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
???
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, thanks - I intended to put here:
'ccmake .. -DCMAKE_INSTALL_PREFIX='${YOUR_KOKKOS_TOOLS_INSTALL_DIR}'
By the way, just doing ccmake ..
and not putting the install prefix may still work (by luck) on some platforms/environments, but we ought to highly recommended Kokkos Tools users to not do this. Maybe we can go into that detail in more detailed documentation in a section like "Dos and Don'ts".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was not going to get into it because it is outside the scope of the PR but using ccmake
isn't exactly the first thing we should showcase. It may be mentioned as a possibly convenient way to discover available options but a plain vanilla build (invoke cmake without any extra option besides installation prefix) will be just right in the vast majority of cases. The ccmake
interface will overwhelm beginners and introduce unnecessary cognitive load for new users.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am NOT suggesting you fix it here (please don't) just pointing out this is a poor choice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it
@dalg24 I just now have fixed the missing install directory in the build instructions and to make it auditable. I think no other unrelated changes are in this PR. Note that the only change should be in the build and run instructions, where I have pulled up the build instructions I had at the top and just removed the build instructions with kp_memory_events. This PR is using a branch on Kokkos Tools. In principle, I should be creating a branch on my fork to merge. However, I want this branch to be visible to users since it is a quick but useful fix and so they know the README is currently being updated. |
@masterleinad Please feel free to comment here also. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What exactly is the fix here? What was broken?
### Run | ||
|
||
Given your installed tool shared library `lib<name_of_tool_shared_lib>.so` and an application executable called yourApplication.exe, type: | ||
|
||
`export KOKKOS_TOOLS_LIBS=${YOUR_KOKKOS_TOOLS_INSTALL_DIR}/lib<name_of_tool_shared_lib>.so; ./yourApplication.exe` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no reason to distinguish between Makefiles and CMake for running.
### Run | |
Given your installed tool shared library `lib<name_of_tool_shared_lib>.so` and an application executable called yourApplication.exe, type: | |
`export KOKKOS_TOOLS_LIBS=${YOUR_KOKKOS_TOOLS_INSTALL_DIR}/lib<name_of_tool_shared_lib>.so; ./yourApplication.exe` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a subtle difference. Maybe that should be changed in the build, but that's another PR.
The difference is that Makefiles generate <name_of_tool_shared_lib>.so
in the source directory of the tool connector, while the cmake generates a library lib<name_of_tool_shared_lib>.so
in the install directory.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Homogenizing the library names sounds like worth fixing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Homogenizing the library names sounds like worth fixing.
I agree with you and thanks for that feedback.
This would be a separate PR and is outside the scope of this PR (if you disagree, let me know).
Note the updated and more specific goal of this PR that I put in the description, so it is clear what needs to be fixed in this PR.
Co-authored-by: Daniel Arndt <[email protected]>
Co-authored-by: Daniel Arndt <[email protected]>
See edited description as noted in another reply to your review. |
Hi, just pinging here. I have addressed the comments to this PR and all is done from my end. Please let me know if this can reviewed and merged if it looks good to you, and if possible by Thursday. Thank you! |
Co-authored-by: Daniel Arndt <[email protected]>
Revise to improve build and run instructions. Specifically, the current build instructions in the general usage section have the wrong path and it also is inconsistent (outdated) w.r.t. the detailed build instructions. This is based on a user discussion on Slack.
The fix here is to consolidate general usage and build and run instructions to remove and put that information at the top.
The current build instructions in the general usage section have the wrong path.